Skip to content

Conversation

@marcusDenslow
Copy link

Summary

Replace panic-prone .unwrap() calls with defensive error handling in the build
helper crate.

Problem

The build system could panic on malformed input from:

  • Git commands returning unexpected output format
  • Missing or unreadable .gitmodules files
  • Invalid stage0 configuration lines

Solution

  • git.rs: Use ? operator for safe git diff-index parsing
  • util.rs: Provide safe fallback for missing .gitmodules files
  • stage0_parser.rs: Skip malformed config lines with warnings

Replace panic-prone .unwrap() calls with defensive error handling
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Sep 17, 2025
@rustbot
Copy link
Collaborator

rustbot commented Sep 17, 2025

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@marcusDenslow marcusDenslow changed the title Fix denial-of-service vulnerabilities in build helper Remove panic-prone .unwrap() calls from build_helper Sep 17, 2025
@marcusDenslow
Copy link
Author

Note: In stage0_parser.rs, malformed config lines are now skipped with a warning instead of panicking. I’m not sure if this should be a hard error instead (fail with a proper message). Feedback on that point would be appreciated.

Comment on lines +42 to +48
let (key, value) = match line.split_once('=') {
Some((k, v)) => (k, v),
None => {
println!("Warning: Skipping malformed config line {}", line);
continue;
}
};
Copy link
Author

@marcusDenslow marcusDenslow Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skipping malformed stage0 config lines with a warning might hide real issues in the user's stage0 config.
Would it be better to fail with a proper error message here instead (e.g. bail! or return Err(...)), so the user knows right away their config is broken?

small note: the warning currently uses println! We might want to change this to eprintln! so that it goes to stderr. open to feedback on whats prefferable

@Mark-Simulacrum
Copy link
Member

IMO, unwrap actually works pretty well for our purposes - it points directly at the code that's failing rather than requiring something else to trace to that location. So I don't actually think we should do something like this.

Thanks!

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 21, 2025
@marcusDenslow marcusDenslow deleted the fix-build-helper-panics branch September 23, 2025 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants